Amber Code Review — feat(security): SSO/JWT authentication migration (Phase 1)

Summary

Substantial, well-structured SSO migration. The core auth path — JWKS-based JWT validation, K8s impersonation, dual-path (JWT + TokenReview) fallback, and encrypted httpOnly session cookie — is architecturally sound and the approach is correct. One critical security flaw in the OIDC callback (open redirect) must be fixed before merge, along with a few major issues around the Keycloak proxy surface and committed credential placeholders.

Findings

Critical

Open redirect in OIDC callback — components/frontend/src/app/api/auth/sso/callback/route.ts

const returnTo = cookieStore.get("oidc_return_to")?.value || "/";
// ...
return NextResponse.redirect(new URL(returnTo, origin));

new URL(absoluteUrl, base) ignores base when the first argument is already absolute. An attacker can craft /api/auth/sso/login?returnTo=https://evil.com; the server stores it in the httpOnly oidc_return_to cookie; after successful OIDC login the user is silently redirected to evil.com. Classic phishing vector.

Fix: validate returnTo is a relative path before storing/using it:

function safeReturnTo(value: string | undefined): string {
  if (!value) return "/";
  try {
    const url = new URL(value, "http://localhost");
    // Reject if the input was already absolute (has its own host)
    if (new URL(value, "http://localhost").origin !== "http://localhost") return "/";
    return url.pathname + url.search;
  } catch { return "/"; }
}

Apply in both login/route.ts (before storing to cookie) and callback/route.ts (before using from cookie).

Major

1. CHANGE_ME placeholders committed to cluster overlay — components/manifests/overlays/hcmais/jsell-sso-poc/

sso-credentials.yaml has SSO_CLIENT_SECRET: CHANGE_ME, SESSION_SECRET: CHANGE_ME. keycloak-admin-secret.yaml has password: CHANGE_ME. keycloak-db.yaml has POSTGRES_PASSWORD: CHANGE_ME. keycloak-realm.json has "clientSecret": "CHANGE_ME" for both the frontend client and the RH SSO identity provider.

These are in a cluster-specific overlay intended for production (hcmais). If applied without rotation the session cookie encryption key is trivially known. Should either:

• Add a # REQUIRED: rotate before applying comment block and a pre-flight check in the deployment workflow
• Or use ExternalSecret / SealedSecret so real values are never committed

The Kind overlay correctly uses dev-secret-do-not-use-in-prod labels and is isolated to dev. The hcmais overlay needs the same level of care.

2. Unrestricted Keycloak proxy — components/frontend/src/app/sso/[...path]/route.ts

This route proxies all GET and POST requests to SSO_ORIGIN (Keycloak) with no path allowlisting. It exposes every Keycloak HTTP endpoint to the public internet via the frontend, including the admin REST API (/auth/admin/...), token introspection, user endpoints, etc. Keycloak has its own auth on admin routes, but this needlessly widens the attack surface and makes ingress WAF rules harder.

Suggested: restrict to paths the OIDC flow actually needs:

const ALLOWED_PATHS = /^(\/realms\/[^/]+\/(protocol\/openid-connect|\.well-known))/;
if (!ALLOWED_PATHS.test(`/${path}`)) {
  return NextResponse.json({ error: "Not found" }, { status: 404 });
}

3. E2E auth bypass reachable with SSO enabled on Kind — components/manifests/overlays/kind/frontend-test-patch.yaml

E2E_TEST_HELPERS=true is set unconditionally in the Kind test patch. The e2e-login route's second guard (AMBIENT_ENV === "production") is only meaningful if production overlays explicitly set AMBIENT_ENV=production on the frontend deployment. The Kind overlay doesn't set it, so after make kind-sso-toggle (SSO on), the auth bypass endpoint is live at /api/auth/sso/e2e-login for anyone who can reach the Kind cluster and knows the route exists.

Dev-only cluster, so production impact is nil. But a developer running Kind SSO mode with external access (e.g., port-forwarded to a shared network) is exposed. Consider removing E2E_TEST_HELPERS from the standard Kind patch and requiring it to be set via a separate make kind-e2e-mode toggle, or add AMBIENT_ENV=development to the route guard so the check is explicit both ways.

Minor

4. Module-level OIDC config cache — components/frontend/src/lib/oidc.ts:5-9

cachedConfig and cachedAt are module-level variables. In serverless / Next.js edge deployments, module state is not guaranteed to persist across invocations, which could cause repeated OIDC discovery on every request. Consider using lru-cache or an explicit cache tied to the Next.js request lifecycle, or document that this is intentional for long-lived Node.js processes only.

5. Silent session destroy on missing refresh token — components/frontend/src/lib/session.ts:60-63

When the access token is expired and there's no refresh token, session.destroy() is called silently. No log entry makes it hard to debug "why did my session disappear?" Add: console.warn("SSO: session expired with no refresh token, destroying").

6. Kind Keycloak SecurityContext — components/manifests/overlays/kind/keycloak-deployment.yaml:16

runAsNonRoot: false for Kind Keycloak and Kind api-server (kind/api-server-security-patch.yaml). CLAUDE.md mandates runAsNonRoot on all containers. Dev-only, so no prod risk, but worth noting the deliberate divergence with an inline comment explaining why Keycloak requires it (# Keycloak upstream image requires root; acceptable in Kind dev only).

7. Empty smoke test — components/ambient-api-server/test/integration/integration_test.go:258-261

TestServerStarts has no assertions — the comment explains the intent, but this pattern is fragile: a future refactor that skips TestMain would leave a silently-passing test. Consider require.NotNil(t, helper.AppConfig()) or equivalent to make the assertion explicit.

Positive Highlights

• JWT validator is well-built: JWKS cache with 5-min min-refresh, proper issuer/audience/expiration validation, manual issuer check to support alt-issuers (dev vs. public URL). The validator_test.go is thorough — expired, tampered signature, wrong issuer/audience, malformed, empty, minimal claims — all covered.
• K8s impersonation approach is the right call: no cluster OIDC federation required, all existing RBAC bindings continue to work, SA tokens (API keys) accepted via TokenReview fallback. Identity claim resolution order (preferred_username > email > sub) is consistent across buildImpersonatingClients, setIdentityFromClaims, and getUserSubjectFromContext.
• SSAR cache key improved from raw bearer token to OIDC sub — reduces cache churn on token rotation and is more correct semantically.
• InitJWTValidator non-fatal design: server starts normally without SSO configured; the feature flag is also off in that state. Clean.
• Session cookie correctly configured: httpOnly, SameSite=lax, encrypted via iron-session.
• PKCE+state implemented correctly for Authorization Code Flow.
• Migration ID change well-reasoned and idempotency analysis is rigorous.
• forwardedIdentityMiddleware falls through to legacy header-based path on JWT failure, preserving backward compatibility.

Recommendations (priority order)

1. Fix open redirect in callback/route.ts and login/route.ts — validate returnTo is relative before storing/using. (~10 lines)
2. Add path allowlist to sso/[...path]/route.ts — block everything except /realms/*/protocol/openid-connect/* and /.well-known/*.
3. Address CHANGE_ME placeholders in hcmais/jsell-sso-poc/ — add deployment workflow gate or switch to ExternalSecret/SealedSecret.
4. Document / isolate E2E_TEST_HELPERS — avoid having it enabled by default on the Kind overlay when SSO is toggled on.
5. Items 4–7 are quality improvements that can be addressed in follow-up tickets.

Review by Amber · standards refs: specs/standards/security/security.spec.md, skills/amber-review/SKILL.md, CLAUDE.md

Re-review: SSO/JWT Authentication Migration (Phase 1)

Holistic re-review after fixup commit 1d194b00. Previous review posted 2026-05-21.

Fixes verified ✅

All six items from the initial review were addressed in the fixup commit. Each one is correctly implemented:

Open redirect in callback/login routes — safeReturnTo() now resolves all inputs against http://localhost and rejects anything whose origin differs from that base. Tested mentally against the classic attack vectors: https://evil.com → origin mismatch → /; //evil.com/path → resolves to http://evil.com/path → origin mismatch → /; javascript:alert(1) → URL parse throws → catch → /; /projects/foo → same origin → /projects/foo. Fix is correct.

Keycloak proxy path allowlist — regex /^realms\/[^/]+\/(protocol\/openid-connect|\.well-known|login-actions|resources)\b/ blocks everything except the four legitimate Keycloak endpoint families. Correct.

E2E auth bypass double guard — now requires both E2E_TEST_HELPERS === "true" and AMBIENT_ENV !== "production" before the bypass activates. Returns 404 otherwise. Correct.

CHANGE_ME comment on sso-credentials.yaml — header comment added, clearly marking placeholder values and linking to the deploy workflow. Correct.

runAsNonRoot in Kind overlay — inline comment explains why root is acceptable for the Keycloak upstream image in a dev-only context. Correct.

Integration test guard — TestServerStarts now fails fast with a clear message if TestMain didn't initialize the helper. Correct.

Remaining gaps ⚠️

1. CHANGE_ME comment missing from three sibling overlay files (minor → should fix)

sso-credentials.yaml got the # REQUIRED: Replace CHANGE_ME values before deploying header, but the other three files in the same overlay directory did not:

• components/manifests/overlays/hcmais/jsell-sso-poc/keycloak-admin-secret.yaml
• components/manifests/overlays/hcmais/jsell-sso-poc/keycloak-db.yaml
• components/manifests/overlays/hcmais/jsell-sso-poc/keycloak-realm.json

All three contain literal placeholder strings that would break a real deployment if applied as-is. Same fix needed: add the required-replace comment at the top of each file.

2. E2E_TEST_HELPERS=true unconditional in Kind patch (minor → should fix)

components/manifests/overlays/kind/frontend-test-patch.yaml still sets E2E_TEST_HELPERS=true unconditionally. The E2E route is now double-guarded in code, but anyone running the Kind overlay gets an always-on test-helper surface. This should either be removed from the base Kind patch and injected only in CI test runs, or at minimum carry a comment explaining the intent. As it stands, any developer who applies the Kind overlay locally activates the bypass endpoint.

New bug: session.idToken is never stored 🐛 (must fix before merge)

logout/route.ts reads session.idToken to build the OIDC end_session_endpoint URL:

// logout/route.ts
const endSessionUrl = await getEndSessionUrl(
  session.idToken,   // ← always undefined
  redirectUrl,
);

getEndSessionUrl uses idToken as the id_token_hint parameter, which tells the identity provider which session to terminate. If it is undefined, the function falls back to passing only client_id, which means Keycloak may not terminate the upstream SSO session — only the local cookie is destroyed. Users will find themselves re-logged-in silently the next time they visit, which defeats single sign-out.

The root cause is in callback/route.ts. exchangeCode() returns idToken, but the callback only persists three of the four fields:

// callback/route.ts (current)
const tokens = await exchangeCode(code, codeVerifier, redirectUri);
session.accessToken  = tokens.accessToken;
session.refreshToken = tokens.refreshToken;
// session.idToken is dropped here   ← bug
session.expiresAt    = tokens.expiresAt;
await session.save();

Fix is one line:

session.idToken      = tokens.idToken;

This must be added before session.save() in the callback route.

Style nit (optional)

safeReturnTo() is copy-pasted identically into both login/route.ts and callback/route.ts. It should live in a shared module (e.g. src/lib/auth-utils.ts) and be imported from both routes. Not a blocker, but worth cleaning up before this ships to production to avoid the two copies drifting.

Verdict

The fixup commit is a solid response to the initial review — all six critical and major issues are correctly addressed. Two minor gaps remain (CHANGE_ME comments on sibling files, unconditional E2E_TEST_HELPERS in Kind patch), and there is one new must-fix bug: session.idToken is never persisted to the iron-session cookie, which silently breaks OIDC single sign-out.

Request changes on the idToken omission. The other items can be addressed in the same commit. Happy to re-review once those three lines are in.

Third Pass — Thorough Line-by-Line Review

Reviewed after commit 05bb922e. All three previous fixup commits now absorbed.

All prior issues confirmed resolved ✅

Every finding from reviews 1 and 2 is correctly addressed. I walked each file end-to-end below; I won't rehash what's already fixed.

New findings

MAJOR — hcmais Keycloak realm missing groups scope

File: components/manifests/overlays/hcmais/keycloak/keycloak-realm.json

The Kind realm correctly defines a realm-level groups clientScope with an oidc-group-membership-mapper, and all three clients include it in defaultClientScopes. The hcmais realm does neither:

Kind realm:    ambient-frontend defaultClientScopes: ['openid', 'email', 'profile', 'groups'] ✅
hcmais realm:  ambient-frontend defaultClientScopes: ['openid', 'email', 'profile']         ✗

There is no groups clientScope definition at the realm level and no per-client protocol mapper for group membership. Tokens issued by the hcmais Keycloak will have no groups claim.

In buildImpersonatingClients (handlers/sso.go):

groups := claims.Groups
if len(groups) == 0 {
    groups = []string{"system:authenticated"}
}

Every SSO user on hcmais will be impersonated as system:authenticated and nothing else. Any K8s RoleBinding bound to a specific group (e.g., an LDAP-synced group like ambient-users) will not be honoured. Users may see unexpected 403s on resources gated by group membership.

Fix: Add the groups clientScope to the hcmais realm, mirroring the Kind config:

"clientScopes": [
  {
    "name": "groups",
    "protocol": "openid-connect",
    "protocolMappers": [{
      "name": "groups",
      "protocolMapper": "oidc-group-membership-mapper",
      "config": {
        "full.path": "false",
        "access.token.claim": "true",
        "id.token.claim": "true",
        "claim.name": "groups"
      }
    }]
  }
]

And add "groups" to ambient-frontend.defaultClientScopes.

MAJOR — Identity claim precedence is inconsistent across the stack

There are three places that resolve "who is this user"; all three use a different priority order:

K8s impersonation uses preferred_username as the primary. The context userID uses email as the primary. For any user that has both claims (the common case), the K8s identity is their preferred_username (e.g. jsell) but the application userID is their email (e.g. jsell@redhat.com). These two values are used in different places — userID for secret key naming, credential lookup; K8s identity for RBAC. If any RBAC RoleBinding is authored with the email address as the subject, it would match K8s impersonation only when preferred_username is absent. If it is authored with the username, userID-keyed lookups won't match.

The root fix is to make setIdentityFromClaims use preferred_username first for userID, matching the impersonation order:

// Consistent with buildImpersonatingClients
primaryID := claims.PreferredUsername
if primaryID == "" {
    primaryID = claims.Email
}
if primaryID == "" {
    primaryID = claims.Sub
}
c.Set("userID", SanitizeUserID(primaryID))
c.Set("userIDOriginal", primaryID)

MINOR — SSO middleware falls through to X-Forwarded-* headers after JWT failure

File: components/backend/server/server.go, forwardedIdentityMiddleware

When SSO is enabled and JWT validation fails (e.g., the request carries an API key that is not a Keycloak JWT), the middleware logs the failure and falls through to the legacy OAuth proxy path:

// JWT validation failed — fall through to header-based extraction
// (API keys will be handled by getK8sClientsDefault via TokenReview)
// [falls through to reading X-Forwarded-User, X-Forwarded-Email, etc.]

The comment is correct — API key identity is re-established by setIdentityFromTokenReview inside getK8sClientsDefault, which overwrites whatever the middleware set. So in practice no handler sees wrong identity. But the middleware shouldn't trust X-Forwarded-* headers at all when SSO is the active auth mode — those headers belonged to the OpenShift OAuth proxy era and should be treated as untrusted in the SSO path.

The minimal fix is a guarded return:

if SSOEnabled() {
    // JWT failed AND we're in SSO mode — don't fall through to OAuth proxy headers.
    // Identity will be established by tokenReviewIdentity in getK8sClientsDefault.
    c.Next()
    return
}

This is defense-in-depth: it closes the theoretical path where a direct-to-backend caller sets crafted X-Forwarded-* headers alongside an API key token.

MINOR — OIDC authorization request only asks for openid scope

File: components/frontend/src/lib/oidc.ts, buildAuthorizationUrl

scope: "openid",

The code later depends on email and preferred_username claims. These are delivered by Keycloak because email and profile are in the client's defaultClientScopes, but the request doesn't ask for them explicitly. If the Keycloak admin ever changes the default scopes, the claims silently disappear and auth degrades in a hard-to-diagnose way. Should be:

scope: "openid email profile",

MINOR — getAccessToken() doesn't persist refreshed idToken

File: components/frontend/src/lib/session.ts

refreshOIDCTokens returns a new idToken (Keycloak always issues a new one on refresh). The refresh path saves accessToken, refreshToken, and expiresAt but drops idToken:

session.accessToken  = tokens.accessToken;
session.refreshToken = tokens.refreshToken;
// session.idToken not updated here
session.expiresAt    = tokens.expiresAt;

The old idToken from initial login continues to work as id_token_hint at logout (Keycloak accepts it), so this isn't broken today. But Keycloak rotates session keys on refresh, and some configurations invalidate old ID tokens. Add session.idToken = tokens.idToken to the refresh block for correctness.

MINOR — ambient-cli client lacks an audience mapper

File: components/manifests/overlays/hcmais/keycloak/keycloak-realm.json (and Kind equivalent)

The ambient-frontend client has a protocol mapper that adds ambient-frontend to the aud claim of access tokens. The ambient-cli client has no such mapper.

The backend validates JWT audience against SSO_AUDIENCE (which is ambient-frontend). A user who authenticates via the CLI client will receive a token without aud: ambient-frontend, and the backend will reject it with a validation error. If the CLI is intended to reach the backend in Phase 1 (or Phase 2), this needs an audience mapper on the CLI client. If the CLI is out of scope for now, add a comment in the realm JSON noting this.

MINOR — new URL(SSO_ISSUER_URL) at module init in Keycloak proxy

File: components/frontend/src/app/sso/[...path]/route.ts

const SSO_ORIGIN = process.env.SSO_ISSUER_URL
  ? new URL(process.env.SSO_ISSUER_URL).origin
  : null;

This executes at module load time. If SSO_ISSUER_URL is set to a malformed value (missing protocol, trailing space, etc.), new URL() throws a TypeError that propagates as an unhandled module-load error, crashing the Next.js worker. Wrap it:

function parseSSOOrigin(): string | null {
  try {
    return process.env.SSO_ISSUER_URL
      ? new URL(process.env.SSO_ISSUER_URL).origin
      : null;
  } catch {
    console.error('SSO_ISSUER_URL is not a valid URL:', process.env.SSO_ISSUER_URL);
    return null;
  }
}
const SSO_ORIGIN = parseSSOOrigin();

Nits

Keycloak proxy Location rewrite uses String.replace (first match only):

value.replace(SSO_ORIGIN, request.nextUrl.origin + "/sso")

String.replace with a string argument replaces only the first occurrence. Use replaceAll or a global regex. Unlikely to matter in practice (Location headers don't repeat the origin), but correct API is replaceAll.

hcmais realm client secret in keycloak-realm.json is CHANGE_ME:

If the realm JSON is imported into Keycloak as-is (which the --import-realm flag does on startup), the ambient-frontend Keycloak client secret will be set to CHANGE_ME, separate from the SSO_CLIENT_SECRET in the K8s secret. Both must be updated together. The kustomization now has a comment about this; consider adding a line specifically calling out the realm JSON client secret requirement: "secret": "<must match SSO_CLIENT_SECRET>".

Overall assessment

The core implementation is solid. The JWT validator, JWKS cache, K8s impersonation, BFF OIDC session model, and E2E bypass guards are all well-designed. The two major findings (groups scope gap and identity precedence inconsistency) should be addressed before this goes to production — they don't block the PoC deployment but will cause real RBAC failures when users with group-based access controls hit the system.

The five minor items are fixes, not blockers for the PoC. Requesting changes on the groups scope and identity precedence; everything else is advisory.